[test-improver] Improve tests for auth package#271
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves test coverage and quality for the internal/auth package by converting manual error checking to testify assertions and adding 14 new edge case tests. The changes enhance test maintainability while increasing test coverage from approximately 85% to 95%.
Changes:
- Migrated all manual
if got != wantchecks to testify assertions (assert.Equal,require.ErrorIs,require.NoError) - Added 14 new edge case tests covering Unicode, special characters, whitespace variations, case sensitivity, and boundary conditions
- Improved test readability and failure diagnostics through idiomatic Go testing patterns
Comments suppressed due to low confidence (2)
internal/auth/header_test.go:125
- Similar to the Bearer case, this test expects a leading space when "Agent agent-id" is parsed. The implementation uses
strings.TrimPrefix(authHeader, \"Agent \")which would leave " agent-id" (one leading space). This documents potentially problematic behavior where agent IDs with leading whitespace could cause validation issues. Consider whether the implementation should trim whitespace after prefix removal.
wantAPIKey: " agent-id",
wantAgentID: " agent-id",
internal/auth/header_test.go:153
- This test expects trailing whitespace to be preserved in the extracted token. Trailing whitespace in authentication credentials is typically a source of bugs and should be trimmed. Consider whether the implementation should normalize whitespace in extracted values to prevent authentication failures due to accidental whitespace.
wantAPIKey: "my-token ",
wantAgentID: "my-token ",
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| wantAPIKey: " my-token", | ||
| wantAgentID: " my-token", |
There was a problem hiding this comment.
The expected values include a leading space, but the implementation uses strings.TrimPrefix(authHeader, \"Bearer \") which removes only the prefix "Bearer " (with a single space). With input "Bearer my-token" (two spaces), the result will be " my-token" (one leading space), which matches the expectation. However, this test documents unexpected behavior - tokens/keys with leading whitespace are likely unintended and could cause authentication failures. Consider whether the implementation should trim all leading/trailing whitespace after prefix removal.
This issue also appears in the following locations of the same file:
- line 124
| wantAPIKey: " ", | ||
| wantAgentID: " ", | ||
| wantErr: nil, |
There was a problem hiding this comment.
This test expects whitespace-only headers to be treated as valid authentication. However, this is problematic from a security perspective - whitespace-only values should likely be rejected or treated as empty. The implementation at line 59 only checks for authHeader == \"\", allowing whitespace-only strings to be used as API keys/agent IDs, which could lead to security issues.
This issue also appears in the following locations of the same file:
- line 152
| wantAPIKey: " ", | |
| wantAgentID: " ", | |
| wantErr: nil, | |
| wantAPIKey: "", | |
| wantAgentID: "", | |
| wantErr: ErrMissingAuthHeader, |
Test Improvements: header_test.go
File Analyzed
internal/auth/header_test.gointernal/authImprovements Made
1. Better Testing Patterns ✅
if got != wantblocks withassert.Equal()require.ErrorIs()andrequire.NoError()assertandrequirepackagesrequire.ErrorIs()for proper error comparison vs simple equalityBefore (Manual Checking):
After (Testify Assertions):
2. Increased Coverage ✅
TestSanitizeForLogging: 6 → 9 test cases (+3)
"key-with-émojis-🔑""key!@#$%^&*()"TestParseAuthHeader: 4 → 11 test cases (+7)
"Bearer my-token""bearer my-token""Agent agent-id"" ""key!@#$%^&*()""Bearer my-token "TestValidateAPIKey: 4 → 8 test cases (+4)
provided="", expected="""My-Secret-Key"vs"my-secret-key""key with spaces""my-key "vs"my-key"Coverage Improvement:
3. Cleaner & More Stable Tests ✅
requirefor critical checks,assertfor comparisonsTest Execution
Branch:
test-improver/auth-header-testsAll changes are backwards compatible and additive. The test structure remains table-driven, maintaining consistency with existing patterns.
Why These Changes?
Selection Rationale
internal/auth/header_test.gowas selected as the #1 candidate from test file analysis because:go.mod, this file used NO assertionsif got != wantblocksImpact
Edge Cases Matter
The new edge case tests catch real-world scenarios:
Generated by Test Improver Workflow
Focuses on better testify usage, increased coverage, and cleaner test patterns